Skip to content

Fix #50 - Breaking: refactory of all main/worker hooks #58

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 25, 2023
Merged

Fix #50 - Breaking: refactory of all main/worker hooks #58

merged 1 commit into from
Oct 25, 2023

Conversation

WebReflection
Copy link
Contributor

@WebReflection WebReflection commented Oct 6, 2023 •
edited
Loading

This MR implements changes discussed in #50 and by doing so, it breaks everything that to date used hooks.

Breaking Change

The option object now accepts a hooks field with the following properties:

  • main
    • onReady
    • onWorker - triggered on main, it's the current onWorkerReady without the ready part as that's misleading
    • onBeforeRun
    • onBeforeRunAsync
    • onAfterRun
    • onAfterRunAsync
    • codeBeforeRun - exact same as within workers
    • codeBeforeRunAsync - exact same as within workers
    • codeAfterRun - exact same as within workers
    • codeAfterRunAsync - exact same as within workers
  • worker
    • onReady - as serializable standalone callback
    • onBeforeRun - as serializable standalone callback
    • onBeforeRunAsync - as serializable standalone callback
    • onAfterRun - as serializable standalone callback
    • onAfterRunAsync - as serializable standalone callback
    • codeBeforeRun edit NOTE: these are all strings, not callbacks
    • codeBeforeRunAsync
    • codeAfterRun
    • codeAfterRunAsync

All hooks aer already tested via previous (and modified) integration tests.

@tedpatrick
Copy link

tedpatrick commented Oct 6, 2023 •
edited by WebReflection
Loading

We need defensive logic for plugin hooks.

If/When plugin hooks fault:

@WebReflection
Copy link
Contributor Author

WebReflection commented Oct 6, 2023 •
edited
Loading

@tedpatrick thanks for chiming in ... some extra thoughts of mine:

Do they crash the whole system?

arguably they probably should ... I can imagine plugins depending on other plugins or plugins that grant some behavior (PyScript error to name the fist one we have) ... should the rest of the App work knowing that essential plugin is broken? I am not entirely sure here ... but a good error on what broke seems fair, although hooks don't carry the plugin name, so that's eventually a PyScript feature to implement, still a valid one.

@WebReflection WebReflection force-pushed the issue-50-ok branch 3 times, most recently from a8eb83b to 5c9ff62 Compare October 11, 2023 08:39
@WebReflection WebReflection marked this pull request as ready for review October 11, 2023 08:49
@WebReflection
Copy link
Contributor Author

OK, all details and rebases are just fine now and all better errors will be moved into PyScript as it's PyScript that invokes hooks knowing the exact name and callback, see https://github.com/pyscript/pyscript/blob/main/pyscript.core/src/core.js#L49-L53

@WebReflection WebReflection force-pushed the issue-50-ok branch 2 times, most recently from 91a975f to 3a94107 Compare October 11, 2023 15:23
This was referenced Oct 11, 2023
Closed
Closed
@WebReflection WebReflection mentioned this pull request Oct 16, 2023
Closed
@WebReflection WebReflection force-pushed the issue-50-ok branch 3 times, most recently from b6e9143 to c7f87d8 Compare October 25, 2023 13:09
ntoll
ntoll approved these changes Oct 25, 2023
View reviewed changes
Copy link
Member

@ntoll ntoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@ntoll
Copy link
Member

ntoll commented Oct 25, 2023

I'll put together a WiP PR over in https://github.com/pyscript/docs to document these hooks. We can refine a bunch of stuff about how we want to document plugins there too.

@WebReflection
Copy link
Contributor Author

@ntoll I've just updated the docs too to explain hooks the best I could.

@ntoll
Copy link
Member

ntoll commented Oct 25, 2023

@WebReflection great stuff and thank you!

@WebReflection WebReflection force-pushed the issue-50-ok branch 3 times, most recently from 11bd845 to d7774e3 Compare October 25, 2023 14:43
@WebReflection WebReflection merged commit 87dc308 into main Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ntoll ntoll ntoll approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@WebReflection @tedpatrick @ntoll